Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a paragraph about self #8928

Merged
merged 16 commits into from
Dec 26, 2024
Merged

Conversation

elenakrittik
Copy link
Contributor

@elenakrittik elenakrittik commented Feb 8, 2024

Adds a paragraph about the self keyword and documents that it can be used to refer to variables
defined in subclasses of current class.

Closes godotengine/godot#87944

@AThousandShips AThousandShips added enhancement topic:gdscript area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Feb 8, 2024
@AThousandShips AThousandShips requested a review from a team February 8, 2024 18:11
@dalexeev
Copy link
Member

dalexeev commented Feb 9, 2024

Thanks for the PR! This is greatly appreciated as it is one of the oldest undocumented features of GDScript. However, in my opinion, in the current edition the emphasis is shifted a little in a wrong direction. This feature is not intended solely for inheritance, as the reader might think. I think member implies a static context, and self.member implies a dynamic context. For example, you can declare in the same class dynamic properties using _set() and _get(), and to access them you can use set("member", value) and get("member") or self.member, but you can't use just member.

In GDScript there is no _call()/_invoke() method for dynamically defining methods, so the only example is defining a method in a descendant class. Note that often considered bad practice, the base class should not know about of its descendants. It is generally recommended to define an empty ("abstract" or "virtual") method in the base class. But for the purposes of illustrating the self syntax features, we could use an example like:

if has_method("test"):
    test() # Error.
    self.test() # OK.
    call("test") # OK.

@elenakrittik
Copy link
Contributor Author

elenakrittik commented Feb 9, 2024

@dalexeev I (hopefully) addressed most of your concerns in e7e4ac8, but i am still not sure how to approach the examples. I have something like this in mind:

class_name Item extends Node

# Returns the expected size an item will take up in
# player's inventory (or -1 if it cannot be collected).
func get_size() -> int:
	var size = get(&"size")

	return size if size else -1

# When player touches an item, collect it if it is
# collectible (i.e. has a `collect` method).
func on_player_touch() -> void:
	if has_method(&"collect"):
		# collect() # Would be an error!
		# self.collect() # @dalexeev This is also appears to be an error
						 # on latest 4.2, perhaps you misremembered this part?
		call(&"collect")

# .. another file ..

class_name Potion extends Item

var size := 2

func collect() -> void:
	print("Collected a potion!")

But it looks pretty verbose and (in my view) not immediately understandable without additional comments. Do you have any better ideas?

@elenakrittik
Copy link
Contributor Author

elenakrittik commented Mar 2, 2024

@dalexeev As i haven't heard anything negative towards the proposed example, i went ahead and replaced the weapon/pistol example with this new one. I also added a .. warning:: section, and would like to hear your opinion on it, especially the second paragraph (as i am not an expert on GDScript and am not sure what is considered the best solution for each problem).

@Calinou
Copy link
Member

Calinou commented Aug 2, 2024

@elenakrittik Could you look into replying to the above suggestions (or applying them directly) so this PR can be merged? Thanks in advance 🙂

@elenakrittik
Copy link
Contributor Author

Much sorry, i'll apply the automatic change right away but i unfortunately will not be able to address dalexeev's feedback until after a week or so. Once i'm back i'll make sure to drive this to the finish line!

@elenakrittik
Copy link
Contributor Author

Sorry for the long wait! I believe everything should be good now.

@dalexeev
Copy link
Member

set("not_exist", 123) # No error or warning.
self["not_exist"] = 123 # Runtime error.
self.not_exist = 123 # Runtime error.
not_exist = 123 # Compile time error.

print(get("not_exist")) # No error or warning.
print(self["not_exist"]) # Runtime error.
print(self.not_exist) # Runtime error.
print(not_exist) # Compile time error.

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If dalexeev approves it's good.

I suggested a couple changes to better follow the style guide.

tutorials/scripting/gdscript/gdscript_basics.rst Outdated Show resolved Hide resolved
tutorials/scripting/gdscript/gdscript_basics.rst Outdated Show resolved Hide resolved
@elenakrittik
Copy link
Contributor Author

Is there someone "final" i should request a review from, or does the "review queue" move automatically?

@tetrapod00
Copy link
Contributor

It's pretty informal for the docs repo, we don't have as much of a process as the main repo does. In most cases PRs are good to merge once it gets an approval from a subject matter expert (dalexeev in this case) and a general docs reviewer (me in this case). Sometimes we merge things with less approvals than that, even, if it's a very obvious change.

This is good to merge I just didn't get around to it yet.

@tetrapod00 tetrapod00 changed the title docs: Add a paragraph about self. Add a paragraph about self. Dec 26, 2024
@tetrapod00 tetrapod00 changed the title Add a paragraph about self. Add a paragraph about self Dec 26, 2024
@tetrapod00 tetrapod00 merged commit 63cec69 into godotengine:master Dec 26, 2024
1 check passed
@tetrapod00
Copy link
Contributor

Merged! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.3 enhancement topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signals' .connect() function's argument (callable) is not type-checked if it is accessed via self..
6 participants